-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add annotations to BundleDeployment
#442
Add annotations to BundleDeployment
#442
Conversation
5aab955
to
7d2a8f7
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #442 +/- ##
==========================================
- Coverage 84.00% 83.79% -0.21%
==========================================
Files 23 23
Lines 844 858 +14
==========================================
+ Hits 709 719 +10
- Misses 93 96 +3
- Partials 42 43 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
6b5528c
to
e30a14f
Compare
sort.SliceStable(resultSet, func(i, j int) bool { | ||
return catalogsort.ByVersion(resultSet[i], resultSet[j]) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we no longer need sorting by version since we filter bundles by bundle version amongst other things. I assume previously we were doing sorting beucase multiple bundles could in theory use the same image (.e.g some sort of fake version).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorting, I think, was to make sure the resolver favored high semver versions over low semver versions. Maybe different context though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For upgrade edges there is a separate sorting below in this file (not in the diff) which is still needed.
This piece of code is where we look for a currently installed bundle by image ref. I suspect that there may be multiple bundles with the same image, but different versions 🤷♂️. So in the current implementation we need sorting to prefer higher version.
But when filtering bundles by of package name, bundle name and bundle version we should be able to uniquely identify a bundle. So I expect there always will be 1 or 0 bundles.
@jmprusi might be able to give some more context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. It looks like we check the result set for 0 bundles, but not more than 1. Can we add another check that errors if there is more than 1 bundle?
// TODO: fast follow - we should check whether we are already supporting the channel attribute in the operator spec. | ||
// if so, we should take the value from spec of the operator CR in the owner ref of the bundle deployment. | ||
// If that channel is set, we need to update the filter above to filter by channel as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is no longer relevant given that we filter by package name, bundle name and bundle version.
bundleName := bundleDeployment.Annotations[AnnotationBundleName] | ||
bundleVersion := bundleDeployment.Annotations[AnnotationBundleVersion] | ||
if pkgName == "" || bundleName == "" || bundleVersion == "" { | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to care about bundle deployments created directly? These annotations will only be present on bundle deployments reconciled by operator-controller.
Edit: I think we do need to care so this whole thing might not fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I don't think we need to care, at least in this context.
I sorta think this context is: is there a BD for this operator already? (And maybe that means we should have a label on the BD like operators.operatorframework.io/operator-name
whose value is Operator.metadata.name
?
Perhaps in the future, there's a separate feature that helps the resolver become aware of other BDs, but I don't think we need to do that now.
e30a14f
to
799c4e9
Compare
"annotations": map[string]string{ | ||
variablesources.AnnotationPackageName: bundle.Package, | ||
variablesources.AnnotationBundleName: bundle.Name, | ||
variablesources.AnnotationBundleVersion: bundleVersion.String(), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make these labels (rather than annotations) so that we can use a label selector to populate our BD cache with just BDs that we are managing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford I think we already do that but in a different way.
We set owner reference on a BD here:
operator-controller/internal/controllers/operator_controller.go
Lines 297 to 306 in 205a487
bd.SetOwnerReferences([]metav1.OwnerReference{ | |
{ | |
APIVersion: operatorsv1alpha1.GroupVersion.String(), | |
Kind: "Operator", | |
Name: o.Name, | |
UID: o.UID, | |
Controller: pointer.Bool(true), | |
BlockOwnerDeletion: pointer.Bool(true), | |
}, | |
}) |
And only reconcile owned objects:
Owns(&rukpakv1alpha1.BundleDeployment{}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kubernetes does not support setting up informers to filter on the server-side using owner references though. So my suggestion is to use labels so that we can also configure the cache to select only BDs that have the expected label set.
This adds annotations to `BundleDeployment` objects created by operator-controller in response to `Operator` objects. This makes it easier to filter bundles during resolution and makes us less dependant on image bundle format. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
799c4e9
to
67ccd0f
Compare
I've been thinking more about this:
I'll bring this up for a discussion and will create a new PR or repoen this one based on the outcome. Closing for now. |
Description
This adds annotations to
BundleDeployment
objects created by operator-controller in response toOperator
objects.This makes it easier to filter bundles during resolution and makes us less dependant on image bundle format.
Reviewer Checklist